[native] Enable node pool type specification when reporting to the coordinator from a C++ worker by jaystarshot · Pull Request #24569 · prestodb/presto

jaystarshot · 2025-02-14T23:36:37Z

Description

Presto supports splitting workers into leaf and non leaf pools. This was added here.

For this the coordinator needs worker-isolation-enabled=true and workers can report their pool_type to the resource manager.

Adding the same configs in presto c++ so that the announcer can report the node type.

With this (since support was already added in coordinator) one can deploy any combination of native/java workers in LEAF/INTERMEDIATE etc.

Motivation and Context

Impact

Test Plan

Tested that setting works and if we set pool_type=LEAF in presto c++ the coordinator will only schedule leaf tasks there

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Enable node pool type specification when reporting to the coordinator from a C++ worker.

steveburnett · 2025-02-17T14:33:50Z

New release note guidelines. Please remove the manual PR link in the following format from the release note entries for this PR.


:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

aditi-pandit · 2025-02-17T21:26:20Z

@jaystarshot : Is the node pool type an optimization or it affects functionality as well ? So if a non-leaf worker received a TableScan would it throw error ?

jaystarshot · 2025-02-18T00:10:38Z

@aditi-pandit The coordinator won’t send a non leaf fragment. If it does i believe the execution should be normal since there is no blocking logic in the worker.
Also if no intermediate or leaf workers are present when isolation is enabled then the query fails with internal cluster is starting errors.

jaystarshot · 2025-02-19T17:39:58Z

@majetideepak can you please take a look

aditi-pandit · 2025-02-20T03:40:28Z

presto-native-execution/presto_cpp/main/PrestoServer.cpp

      address_ = fmt::format("[{}]", address_);
    }
    nodeLocation_ = nodeConfig->nodeLocation();
+    nodePoolType_ = systemConfig->poolType();


What are the possible values for pool Type ? Might be good to validate the value.

The accepted values are here Sure I can add that

aditi-pandit · 2025-02-20T03:45:42Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

  return requiredProperty(std::string(kPrestoVersion));
 }

+std::string SystemConfig::poolType() const {


Wanted to confirm that this is not a registered property... Else it has to be a part of registeredProps_. If you add it to registeredProps_ it might be simpler to initialize the default value as well.

What is the difference between registeredProps_ and configs?

Registered properties are validated on server startup https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/common/Configs.cpp#L110

Might be better to add it to avoid warnings.

aditi-pandit

@jaystarshot : Just had 2 questions else this PR should be fine.

jaystarshot · 2025-02-20T04:46:42Z

@aditi-pandit I am just using the same property name as presto java uses. Its defined in serverconfig here.

aditi-pandit · 2025-02-20T05:22:50Z

@aditi-pandit I am just using the same property name as presto java uses. Its defined in serverconfig here.

Thanks @jaystarshot for the explanation. I saw the linked PR later. Yeah, we'll have to stick with this name.

aditi-pandit · 2025-02-20T17:42:29Z

@pdabre12 @pramodsatya : Can you'll take a look at the failures in prestocpp-linux-build-and-unit-test / prestocpp-linux-presto-sidecar-tests (pull_request)

steveburnett · 2025-02-20T18:41:50Z

Suggest rephrasing of release note entry to follow the Order of changes, and Order of sections grouping in the Release Notes Guidelines:

== RELEASE NOTES ==

Prestissimo (Native Execution) Changes
* Add node pool type specification in Presto C++ for reporting to the coordinator from a C++ worker.

aditi-pandit

@jaystarshot : Have one major comment about the required vs optional. The rest are nits.

Another thing : Please change your commit to add a prefix of [native]

aditi-pandit · 2025-02-20T18:44:20Z

presto-native-execution/presto_cpp/main/common/Configs.cpp


+std::string SystemConfig::poolType() const {
+    static const std::unordered_set<std::string> validTypes = {"LEAF", "INTERMEDIATE", "DEFAULT"};
+    auto value = requiredProperty(std::string(kPoolType));


Should this be optionalProperty instead ? Configs which don't have pool-type will cause failures post this change. So this could be very disruptive. Could that be the reason for all the test failures ? I haven't looked at the details but the timeouts most likely could be because the workers didn't start up.

i thought the default will always be initialized when we put this in the registeredProps_ map i can try with optional

aditi-pandit · 2025-02-20T18:45:26Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

 }

+std::string SystemConfig::poolType() const {
+    static const std::unordered_set<std::string> validTypes = {"LEAF", "INTERMEDIATE", "DEFAULT"};


Nit rename variable to kPoolTypes.

Constant variable names should be prefixed with 'k'

aditi-pandit · 2025-02-20T18:46:24Z

presto-native-execution/presto_cpp/main/Announcer.h

      const bool sidecar,
      const std::vector<std::string>& connectorIds,
      const uint64_t maxFrequencyMs_,
+      const std::string& nodePoolType,


We could move this parameter higher up since there aren't any optional parameters in this method. After nodeLocation or sidecar ?

aditi-pandit

@jaystarshot : Please change your commit title to have prefix [native] as well.

One minor nit otherwise.

aditi-pandit · 2025-02-21T22:10:00Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

 }

+std::string SystemConfig::poolType() const {
+    static const std::unordered_set<std::string> validTypes = {"LEAF", "INTERMEDIATE", "DEFAULT"};


Nit: Rename this set of constants with k Prefix as well. kPoolTypes maybe

aditi-pandit

Thanks for the iterations @jaystarshot

aditi-pandit

Thanks for the test change @jaystarshot

jaystarshot changed the title ~~Support specification of node pool type when announcing cpp worker~~ Support specification of node pool type when announcing to coordinator from c++ worker Feb 14, 2025

jaystarshot changed the title ~~Support specification of node pool type when announcing to coordinator from c++ worker~~ Enable node pool type specification when reporting to the coordinator from a C++ worker Feb 15, 2025

jaystarshot marked this pull request as ready for review February 15, 2025 00:17

jaystarshot requested a review from a team as a code owner February 15, 2025 00:17

jaystarshot requested a review from majetideepak February 15, 2025 00:17

aditi-pandit reviewed Feb 20, 2025

View reviewed changes

aditi-pandit changed the title ~~Enable node pool type specification when reporting to the coordinator from a C++ worker~~ [native] Enable node pool type specification when reporting to the coordinator from a C++ worker Feb 20, 2025

aditi-pandit reviewed Feb 20, 2025

View reviewed changes

jaystarshot force-pushed the jay-leaf branch 2 times, most recently from 7088f2f to bc3388d Compare February 20, 2025 17:07

aditi-pandit reviewed Feb 20, 2025

View reviewed changes

jaystarshot force-pushed the jay-leaf branch from bc3388d to fe3a38b Compare February 21, 2025 18:17

aditi-pandit reviewed Feb 21, 2025

View reviewed changes

jaystarshot force-pushed the jay-leaf branch from fe3a38b to 4573986 Compare February 21, 2025 22:55

aditi-pandit previously approved these changes Feb 21, 2025

View reviewed changes

jaystarshot dismissed aditi-pandit’s stale review via 38b72d5 February 22, 2025 00:19

jaystarshot force-pushed the jay-leaf branch from 4573986 to 38b72d5 Compare February 22, 2025 00:19

[native] Support specification of node pool type in announcer

77690d6

jaystarshot force-pushed the jay-leaf branch from 38b72d5 to 77690d6 Compare February 22, 2025 01:04

aditi-pandit approved these changes Feb 22, 2025

View reviewed changes

jaystarshot merged commit 4b8271c into prestodb:master Feb 22, 2025
59 of 60 checks passed

unidevel mentioned this pull request Mar 10, 2025

Add release notes for 0.292 unix280/presto#5

Closed

20 tasks

unidevel mentioned this pull request Mar 10, 2025

Add release notes for 0.292 unix280/presto#6

Closed

24 tasks

prestodb-ci mentioned this pull request Mar 28, 2025

Add release notes for 0.292 #24825

Merged

30 tasks

unidevel mentioned this pull request Apr 25, 2025

Add release notes for 0.292 unix280/presto#23

Closed

30 tasks

This was referenced May 6, 2025

Add release notes for 0.292 unix280/presto#28

Closed

Add release notes for 0.292 unix280/presto#29

Closed

Conversation

jaystarshot commented Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Feb 17, 2025

Uh oh!

aditi-pandit commented Feb 17, 2025

Uh oh!

jaystarshot commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaystarshot commented Feb 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

jaystarshot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aditi-pandit commented Feb 20, 2025

Uh oh!

aditi-pandit commented Feb 20, 2025

Uh oh!

steveburnett commented Feb 20, 2025

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

aditi-pandit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaystarshot commented Feb 14, 2025 •

edited

Loading

jaystarshot commented Feb 18, 2025 •

edited

Loading

jaystarshot commented Feb 20, 2025 •

edited

Loading